Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vmConfig check for invalid tracer. #27002

Closed
wants to merge 1 commit into from

Conversation

0xTylerHolmes
Copy link
Contributor

When we create a new blockchain we supply a vmConfig object. If we set debug mode to true without supplying aTracer we have the following nil pointer deref.

st.evm.Config.Tracer.CaptureTxStart(st.initialGas)

If we specify debug without adding a tracer we will get the nil pointer deref here. To mitigate this the constructor for blockchain checks that if the vmConfig has debug mode enabled it also checks to make sure the tracer is non-nil.

@rjl493456442
Copy link
Member

rjl493456442 commented Mar 29, 2023

I think we can do it in an alternative approach. We can remove the Debug field in vm.Config, but define Tracing() function(or some other better name) instead. If the tracer is configured, then we dump the execution statistics into tracer.

// Config are the configuration options for the Interpreter
type Config struct {
	// Debug                   bool      // Enables debugging
	Tracer                  EVMLogger // Opcode logger
	NoBaseFee               bool      // Forces the EIP-1559 baseFee to 0 (needed for 0 price calls)
	EnablePreimageRecording bool      // Enables recording of SHA3/keccak preimages
	ExtraEips               []int     // Additional EIPS that are to be enabled
}

// Tracing returns an indicator if evm execution tracer is configured.
func (config *Config) Tracing() bool {
	return config.Tracer != nil
}

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we could just disable Debug and print a warning, but OTOH this is a situation where you probably would rather abort and retry than continue and see a warning.
I'm fine with this. The solution from @rjl493456442 is "not great" IMO, because the Debug check happens in a lot ot hot-spots, where we don't want to incur a method call.

If we switched to use proper constructor-invocations, then it would be easier to solve it by having the Debug flag become set implicitly rather than explicitly.

TLDR; SGTM for now

@s1na
Copy link
Contributor

s1na commented Mar 31, 2023

I'm slightly in favor of dropping Debug as Gary suggested. We can check vmConfig.Tracer != nil to avoid the method invocation overhead.

It seems so far tracer is the only use-case for Debug. Even if it were other cases (e.g. mocking caller as per #26414) they won't be necessarily invoked in the same place as tracing. E.g. see:

https://github.com/ethereum/go-ethereum/pull/26414/files#diff-64508d317d86e7a4a5294d76455a5e74148e26883da9e8b4ffc9bbfa3cc8550eR289-R290

@holiman
Copy link
Contributor

holiman commented Mar 31, 2023

Ok, I'm fine with @rjl493456442 's idea, but then we need to turn it into a local variable in the interpreter loop

@holiman
Copy link
Contributor

holiman commented Apr 4, 2023

I think we can do it in an alternative approach.

Implemented in #27048, PTAL

@holiman
Copy link
Contributor

holiman commented Apr 4, 2023

Closing in favour of #27048

@holiman holiman closed this Apr 4, 2023
@0xTylerHolmes 0xTylerHolmes deleted the vmConfig_check branch April 14, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants